Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] facade pattern - Topic #218

Merged
merged 9 commits into from
Jul 14, 2024
Merged

[FEAT] facade pattern - Topic #218

merged 9 commits into from
Jul 14, 2024

Conversation

kimdozzi
Copy link
Contributor

PR 타입(하나 이상의 PR 타입을 선택해주세요)

☑ 기능 추가

□ 기능 삭제

□ 버그 수정

□ 의존성, 환경 변수, 빌드 관련 코드 업데이트


반영 브랜치

feat/210-facade-pattern -> main


변경 사항

  • Topic Facade Pattern 적용
  • Topic Request/Response Dto 정적 팩토리 메서드 적용
  • Topic Facade(Service), Repository 코드 수정
  • Topic 테스트 코드 Describe-Contex-it(DCI)패턴 도입

테스트 결과

image


image


연관된 이슈

ex) #이슈번호, #이슈번호


리뷰 요구사항(선택)

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
ex) 메서드 XXX의 이름을 더 잘 짓고 싶은데 혹시 좋은 명칭이 있을까요?

kimdozzi added 5 commits July 9, 2024 07:33
- topicController
- topicService
- topicFacade
- topicFacadeImpl
- describe context it 패턴 적용
- facade pattern 적용 후 코드 리펙토링
- describe context it 패턴 적용
- topicServiceTest -> topicFacadeTest로 변경
- facade pattern에 맞게 수정
@kimdozzi kimdozzi linked an issue Jul 10, 2024 that may be closed by this pull request
5 tasks
@kimdozzi kimdozzi requested a review from SSung023 July 10, 2024 01:21
Copy link

github-actions bot commented Jul 10, 2024

Test Results

 61 files   61 suites   25s ⏱️
268 tests 268 ✅ 0 💤 0 ❌
281 runs  281 ✅ 0 💤 0 ❌

Results for commit 9d38cea.

♻️ This comment has been updated with latest results.

@SSung023 SSung023 added ⚙️ refactor 리팩토링 🧑🏻‍💻 BE 백엔드 관련 코드 labels Jul 10, 2024
Copy link
Contributor

@SSung023 SSung023 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파사드 패턴 너무 좋네요! 고생하셨습니다 :)

@@ -13,7 +13,8 @@ public record TopicDetailResponse(
String notice,
int pointPerPerson,
FileResponse fileResponse) {
public static TopicDetailResponse createByEntity(Topic topic, FileResponse fileResponse) {

public static TopicDetailResponse of(Topic topic, FileResponse fileResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오! 정적 팩토리 메서드의 명명법을 따른 네이밍인가보네요! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이펙티브 자바에서 배운 걸 활용해봤습니다 ㅎ

}

@Transactional
public Long create(Topic byTopicCreateDto) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수명이 byTopicCreateDto로 되어 있어 Topic이 아닌 DTO 객체가 들어오는걸로 혼동될 여지가 있어보여요!
범용적으로 사용될 클래스이기도 하고, TopicFacadeImpl에서 create 메서드를 호출할 때에도 이름을 topic으로 전달하니
Long create(Topic topic)는 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 그렇네요! 수정하겠습니다

.pointPerPerson(topicCreateRequest.pointPerPerson())
.notice(topicCreateRequest.notice())
.build();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO <-> Entity 간 변환 코드를 어디에 두느냐가 항상 고민거리가 되는 것 같아요.
저는 DTO -> Entity로 변환하는 코드를 DTO에 넣는 것을 선호하는데요,

  1. DTO의 수가 많아지면 Service 내에 변환하는 코드가 많아져 관리가 힘들어진다
  2. 메서드명을 봤을 때 어떤 역할을 하는지 알 수 있어야 하기 때문에 메서드 명이 길어지는데, DTO의 정적 팩토리 메서드를 사용하는 경우 간략하게 표현할 수 있다.
    예를 들어 TopicCreateRequest의 정적 팩토리 메서드로 사용하는 경우
    기존의 Topic topic = topicService.createTopicByTopicCreateRequest(topicCreateRequest); 에서
    Topic topic = TopicCreateRequest.create(topicCreateRequest);로 표현이 가능할 것 같아요. (앞에 클래스명이 붙어 있기 때문에 create 라는 이름만으로도 충분하다고 판단)

AS-IS:

@RequiredArgsConstructor
@Component
@Transactional
public class TopicFacadeImpl implements TopicFacade {
    ...
    @Override
    public Long create(TopicCreateRequest topicCreateRequest) {
        Topic topic = topicService.createTopicByTopicCreateRequest(topicCreateRequest);
        return topicService.create(topic);
    }
    ...
}

TO-BE:

public record TopicCreateRequest(
        String title,
        String tags,
        String description,
        int pointPerPerson,
        String notice
) {

    public static Topic create(TopicCreateRequest topicCreateRequest) {
        return Topic.builder()
                .title(topicCreateRequest.title())
                .description(topicCreateRequest.description())
                .tags(topicCreateRequest.tags())
                .pointPerPerson(topicCreateRequest.pointPerPerson())
                .notice(topicCreateRequest.notice())
                .build();
    }
}
@RequiredArgsConstructor
@Component
@Transactional
public class TopicFacadeImpl implements TopicFacade {
    ...
    @Override
    public Long create(TopicCreateRequest topicCreateRequest) {
        Topic topic = TopicCreateRequest.create(topicCreateRequest);
        return topicService.create(topic);
    }
    ...
}

한 번 의견 같이 나눠보아요!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO <-> Entity 변환은 DTO에서 처리하는 방식으로 바꿔야겠다고 생각했는데, 예전 습관이 남아 있었는지 자연스레 Service쪽에 뒀네요 ㅎㅎ;;;;; 수정하겠습니다. 좋은 의견 & 예시 감사합니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와... 캬... Facade 패턴을 적용시키니까 훨씬 깔끔하고 가독성도 좋네요...!! 최곱니다 👍👍👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

긴 시간 고민하고 결심한 보람이 있네요 ㅎㅎ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트코드에도 DCI 패턴을 적용하니까 훨씬 덜 복잡하고 보기 좋네요 👍👍👍

}

topic.updateNotExistInstance(topicUpdateRequest.title(), topicUpdateRequest.description(),
topicUpdateRequest.tags(), topicUpdateRequest.notice(), topicUpdateRequest.pointPerPerson());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저 이 부분도 도형님의 의견이 궁금합니다!!!
사실 parameter로 topicUpdateRequest만 전달하고 Topic 클래스 내에서 처리하면 코드도 깔끔하고 좋을 것 같은데,
그렇게 되면 Entity가 DTO를 너무 의존하는 느낌이라 또 고민이 되는 것 같아요🧐

파사드 클래스 내부에서 처리하기로 한 이유가 궁금해용!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음.. 해당 코드에 대해 많이 고민을 해봤는데요.

  1. Controller가 아닌 Facade에서 DTO -> Entity를 변환시킨 이유
    -> 토픽 생성/수정 작업은 어드민 페이지 내에서만 이루어지는 작업이고, 다른 도메인에서 사용할 수 없습니다. 그렇기 때문에 조회/삭제 메서드와 다르게 DTO 자체를 받고 퍼사드 내부에서 처리해도 된다고 판단했습니다.

  2. Facade에서 받은 DTO 객체를 Entity에 넘기지 않고, 퍼사드 내부에서 변환하여 넘겨준 이유
    -> 물론, 생성/수정 작업이 다른 도메인에선 사용되지 않지만, 그렇다고 해서 너무 DTO 자체에 의존하면 안될 것 같다고 판단했습니다. DTO 객체를 Entity에 넘겨주면 DTO를 의존하는 계층이 너무 많아지게 됩니다. Topic Entity는 getter/setter 역할만 충실히 수행하면 되지 않을까 생각합니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오..! 적어주신 답변 내용을 보니 모두 이해가 되네요! 👍
저도 작업할 때 해당 부분 고려하며 코드를 짜봐야겠네요! 좋은 의견 감사합니다! ☺️

kimdozzi added 4 commits July 12, 2024 08:07
- topicService.createTopicByTopic -> TopicCreateRequest.from
- getTopicUpdateRequest 메서드 추출
@kimdozzi kimdozzi merged commit eefe604 into main Jul 14, 2024
2 checks passed
@kimdozzi kimdozzi deleted the feat/210-facade-pattern branch August 25, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑🏻‍💻 BE 백엔드 관련 코드 ⚙️ refactor 리팩토링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] Refactoring - Topic
2 participants